Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add vcomlink test #1016

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Add vcomlink test #1016

merged 3 commits into from
Oct 11, 2024

Conversation

shjala
Copy link
Member

@shjala shjala commented Aug 21, 2024 •
edited
Loading

This is a test for lf-edge/eve#4142, but I don't know which one should go first ?!

@shjala shjala mentioned this pull request Aug 21, 2024
Merged
6 tasks
milan-zededa
milan-zededa reviewed Aug 22, 2024
View reviewed changes
tests/vcom/base.go Outdated Show resolved Hide resolved
milan-zededa
milan-zededa reviewed Aug 22, 2024
View reviewed changes
pkg/remote/utils.go Outdated Show resolved Hide resolved
milan-zededa
milan-zededa reviewed Aug 22, 2024
View reviewed changes
pkg/remote/utils.go Outdated Show resolved Hide resolved
milan-zededa
milan-zededa reviewed Aug 22, 2024
View reviewed changes
pkg/remote/utils.go Outdated Show resolved Hide resolved
@milan-zededa
Copy link
Contributor

This is a test for lf-edge/eve#4142, but I don't know which one should go first ?!

If this test depends on the EVE PR and would fail until it is merged and released, then this PR should wait.
I typically mark such eden PR as draft until there is EVE version with the tested functionality available. Then I update DefaultEVETag in pkg/defaults as part of the PR and mark it as ready for review.

@shjala shjala marked this pull request as draft August 22, 2024 10:20
@shjala
Copy link
Member Author

shjala commented Aug 22, 2024

If this test depends on the EVE PR and would fail until it is merged and released, then this PR should wait.

Yeah, I get it. But it is a conundrum, how I can show that my PR in eve works fine without merging this test, and on the other hand merging this breaks the eden tests.

@shjala shjala force-pushed the vcom_eden_test branch 2 times, most recently from 8d0c3d5 to 06b9eac Compare August 22, 2024 16:23
@shjala shjala mentioned this pull request Aug 23, 2024
Draft
4 tasks
@shjala shjala changed the title add vcomlink test [WIP] add vcomlink test Aug 26, 2024
@shjala shjala force-pushed the vcom_eden_test branch 2 times, most recently from e27c64d to 9962754 Compare August 29, 2024 16:26
@shjala shjala marked this pull request as ready for review September 2, 2024 08:46
@shjala shjala changed the title [WIP] add vcomlink test add vcomlink test Sep 2, 2024
@shjala
Copy link
Member Author

shjala commented Sep 2, 2024

This is ready to be merge as soon as we get the new eve release.

@shjala shjala force-pushed the vcom_eden_test branch 5 times, most recently from f71d237 to 1fd58e3 Compare September 4, 2024 07:35
@shjala shjala marked this pull request as draft September 4, 2024 09:03
@shjala shjala self-assigned this Sep 4, 2024
@shjala shjala force-pushed the vcom_eden_test branch 2 times, most recently from 5cd1830 to c10614b Compare September 13, 2024 10:41
@uncleDecart
Copy link
Member

@shjala so EVE 13.3.0 supports the test?

@shjala
Copy link
Member Author

shjala commented Sep 23, 2024

@shjala so EVE 13.3.0 supports the test?

yes, it runs green locally, lets see what happens here.

@shjala shjala marked this pull request as ready for review September 23, 2024 11:53
@shjala shjala force-pushed the vcom_eden_test branch 3 times, most recently from 655570b to b5dc108 Compare September 24, 2024 14:26
@shjala
Copy link
Member Author

shjala commented Sep 24, 2024 •
edited
Loading

This is really frustrating, it runs fine locally (running smoke test) but here fails to wait for the app to be ready, I increased the timer to 30m but still...

@uncleDecart
Copy link
Member

We need to think if there's something in Eden or problem in runners...

@giggsoff
Copy link
Collaborator

This is really frustrating, it runs fine locally (running smoke test) but here fails to wait for the app to be ready, I increased the timer to 30m but still...

Inside attached artifacts (in console logs) I can see ERROR: App vcomlink-sleepy_leakey uuid 53ff1865-61bf-43dd-bab4-ab7c1ffe2fb6 state INSTALLED error: failed to run domain 53ff1865-61bf-43dd-bab4-ab7c1ffe2fb6.1.1: not based on an OCI image. AFAIK it indicates that you try to run VM (based on qcow2 image) on host without hardware assistance acceleration (or with app in no-hyper mode in config).

BTW it would be nice to show timestanps and errors if any in state transition in test logs (from AppWaitForRunningState function). It will helps to detect such problems quickly without diving into artifacts.

@shjala
Copy link
Member Author

shjala commented Oct 9, 2024 •
edited
Loading

Inside attached artifacts (in console logs) I can see ERROR: App vcomlink-sleepy_leakey uuid 53ff1865-61bf-43dd-bab4-ab7c1ffe2fb6 state INSTALLED error: failed to run domain 53ff1865-61bf-43dd-bab4-ab7c1ffe2fb6.1.1: not based on an OCI image. AFAIK it indicates that you try to run VM (based on qcow2 image) on host without hardware assistance acceleration (or with app in no-hyper mode in config).

I see the eden default DefaultEVEH is KVM and in the app config NoHyper is set to false, and expectation.virtualizationMode = config.VmMode_HVM for amd64, are you suggesting I should move this to virtualization test suite?

BTW it would be nice to show timestanps and errors if any in state transition in test logs (from AppWaitForRunningState function). It will helps to detect such problems quickly without diving into artifacts.

Good point, I will add it.

@shjala
Copy link
Member Author

shjala commented Oct 9, 2024

I see require_virtualization: true only for virtualization test in github actions, I move this test to virt.

@shjala shjala force-pushed the vcom_eden_test branch 2 times, most recently from e0efa23 to 06d3107 Compare October 9, 2024 09:55
@shjala
add AddTimestampf to utils/format
fae2fca 
Signed-off-by: Shahriyar Jalayeri <[email protected]>
@shjala
evetestkit: more verbose
c379865 
AppWaitForRunningState : now reports each state changes
AppWaitForSSH : repoprts wating
EveRebootAndWait : reports waiting
AppSCPCopy : check if local file exists since it is not checked by scp

plus adjusted some sleep times.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
@shjala
Add test for vcomlink
012a6f0 
This is a test for EVE PR #4142.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
@shjala
Copy link
Member Author

shjala commented Oct 9, 2024 •
edited
Loading

@giggsoff Thanks for your help. It is now passing as part of virtualization tests:

--- PASS: TestEdenScripts (0.00s)
    --- PASS: TestEdenScripts/eden_vcom (119.12s)

@milan-zededa @uncleDecart some other tests are failing but I'm not touching any part relevant to those, general flakiness? anyways this PR is ready.

@milan-zededa
Copy link
Contributor

@giggsoff Thanks for your help. It is now passing as part of virtualization tests:

--- PASS: TestEdenScripts (0.00s)
    --- PASS: TestEdenScripts/eden_vcom (119.12s)

@milan-zededa @uncleDecart some other tests are failing but I'm not touching any part relevant to those, general flakiness? anyways this PR is ready.

We will retry them just in case. At least one of them is a known failure.

@shjala
Copy link
Member Author

shjala commented Oct 10, 2024

@milan-zededa @uncleDecart should we merge this?

uncleDecart
uncleDecart approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@uncleDecart uncleDecart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from my side, if @milan-zededa is okay with patch I'd merge it

@uncleDecart uncleDecart merged commit 8545f10 into lf-edge:master Oct 11, 2024
13 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milan-zededa milan-zededa milan-zededa approved these changes

@uncleDecart uncleDecart uncleDecart approved these changes

Assignees

@shjala shjala

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shjala @milan-zededa @uncleDecart @giggsoff